Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix data race in thread::scope #98503

Merged
merged 1 commit into from
Jul 1, 2022
Merged

fix data race in thread::scope #98503

merged 1 commit into from
Jul 1, 2022

Conversation

RalfJung
Copy link
Member

Puts the ScopeData into an Arc so it sticks around as long as we need it.
This means one extra Arc::clone per spawned scoped thread, which I hope is fine.

Fixes #98498
r? @m-ou-se

@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 25, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2022
@danielhenrymantilla
Copy link
Contributor

Hmm, I raced with your PR, @RalfJung 😅: #98504. At least now we can freely pick between the Arc and the UnsafeCell approaches

@@ -33,7 +33,7 @@ pub struct Scope<'scope, 'env: 'scope> {
///
/// See [`Scope::spawn`] for details.
#[stable(feature = "scoped_threads", since = "1.63.0")]
pub struct ScopedJoinHandle<'scope, T>(JoinInner<'scope, T>);
pub struct ScopedJoinHandle<T>(JoinInner<T>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing a lifetime parameter from a stable item is API breaking, is it not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize this is public. oops

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own confidence: PhantomData<&'scope> is the correct existing variance here. (The lifetime variances involved in this API is subtle and controlled by PhantomData, not just normal references.)

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

This looks like a good fix for the issue. We could look into a solution without an Arc later, although I doubt it matters much.

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2022

📌 Commit 846bba4616e70ae9275e947eb450deb7a82f0a79 has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2022
@m-ou-se m-ou-se added this to the 1.63.0 milestone Jun 27, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 27, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

The PhantomData<&'scope T> in ScopedJoinHandle results in slightly different behavior. It's correct for the variance of 'a, but it makes a difference for dropck.

This snippet errors before this change, but compiles after this change:

use std::thread::ScopedJoinHandle;

fn x<'a>(_: &'a i32, h: ScopedJoinHandle<'a, i32>) -> ScopedJoinHandle<'a, i32> {
    h
}

fn main() {
    std::thread::scope(|s| {
        let mut v = Vec::new();
        {
            let a = 1;
            v.push(x(&a, s.spawn(|| 1)));
        }
    });
}

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

(Whether that's a real issue.. I'm not sure yet. But it's a publicly visible change, although an extremely subtle one.)

@RalfJung
Copy link
Member Author

So, was it previously invariant? Or should I replace T by ()?

@RalfJung
Copy link
Member Author

Looking at the old type, the data under lifetime 'scope is ScopeData. So I switched to that type in the PhantomType. I would think that means the old behavior is preserved?

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

Yeah, that sounds right. Testing now..

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

Nope. It still compiles.

@RalfJung
Copy link
Member Author

That... doesn't make any sense, or does it?

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

&'scope ScopeData doesn't have a Drop impl. We need something that has 'scope and might use that lifetime in its Drop impl.

@RalfJung
Copy link
Member Author

The old code doesn't have such a thing either, though?

@m-ou-se
Copy link
Member

m-ou-se commented Jun 27, 2022

It did: JoinInner's drop glue could drop a Packet<'scope, T>, which has a Drop implementation without #[may_dangle].

@RalfJung
Copy link
Member Author

Ah, this should do it. The lifetime needs to be on Packet since that has a impl Drop without may_dangle.

@m-ou-se m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 29, 2022
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as another set of eyes: the current change looks like it should be trivially invisible to public API. The extent of potentially-public-visible changes are:

  • Packet now additionally inherits autotrait constraints from Arc<scoped::ScopeData>.
  • Scope now inherits autotrait constraints from Arc<ScopeData> instead of ScopeData.

ScopeData is Send + Sync + Unpin + UnwindSafe + RefUnwindSafe, thus no public change is visible.


A potential note for the future, though: Scope uses &'a mut &'a mut () to achieve invariance over 'a. This does accomplish this, but it also makes Scope not UnwindSafe.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 30, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? ``@m-ou-se``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 30, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? ```@m-ou-se```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 30, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? ````@m-ou-se````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#97629 ([core] add `Exclusive` to sync)
 - rust-lang#98503 (fix data race in thread::scope)
 - rust-lang#98670 (llvm-wrapper: adapt for LLVMConstExtractValue removal)
 - rust-lang#98671 (Fix source sidebar bugs)
 - rust-lang#98677 (For diagnostic information of Boolean, remind it as use the type: 'bool')
 - rust-lang#98684 (add test for 72793)
 - rust-lang#98688 (interpret: add From<&MplaceTy> for PlaceTy)
 - rust-lang#98695 (use "or pattern")
 - rust-lang#98709 (Remove unneeded methods declaration for old web browsers)
 - rust-lang#98717 (get rid of tidy 'unnecessarily ignored' warnings)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ebecc13 into rust-lang:master Jul 1, 2022
@rustbot rustbot modified the milestones: 1.63.0, 1.64.0 Jul 1, 2022
@RalfJung RalfJung deleted the scope-race branch July 2, 2022 14:13
@ehuss ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 5, 2022
@ehuss ehuss modified the milestones: 1.64.0, 1.63.0 Jul 5, 2022
ehuss pushed a commit to ehuss/rust that referenced this pull request Jul 5, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? `````@m-ou-se`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 6, 2022
[beta] Beta 1.63 backports

* fix data race in thread::scope rust-lang#98503
* Mitigate MMIO stale data vulnerability rust-lang#98126
* Cargo:
    * [BETA-1.63] Fix zsh completions for add and locate-project (rust-lang/cargo#10811)
    * [BETA-1.63] Bump cargo-util version. (rust-lang/cargo#10805)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data race in thread::scope
10 participants